Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: Sync to disk before returning in WaitForApplication #35626

Merged
merged 1 commit into from
Mar 12, 2019

Conversation

bdarnell
Copy link
Contributor

This prevents on-disk inconsistencies when a node crashes in the
middle of a merge and the lease applied index temporarily regresses.

Fixes #33120

Release note (bug fix): Fixed an on-disk inconsistency that could
result from a crash during a range merge.

The second commit is a manual test (adapted from @tbg) that demonstrates the "overlapping range" failure if the WriteSyncNoop call is removed, and also shows that this call fixes the problem. It's obviously not mergeable in its current state, but I'm not sure how to test this without some gross hacks.

@bdarnell bdarnell requested review from tbg and a team March 11, 2019 22:45
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@bdarnell bdarnell force-pushed the merge-waitforapplication branch from 3189799 to 2dcec7d Compare March 12, 2019 16:31
@bdarnell bdarnell changed the title DNM: storage: Sync to disk before returning in WaitForApplication storage: Sync to disk before returning in WaitForApplication Mar 12, 2019
@bdarnell
Copy link
Contributor Author

After consulting with @tbg I've moved the testing code into a separate PR, #35653. Now this commit can go in for the next beta.

@bdarnell bdarnell force-pushed the merge-waitforapplication branch from 2dcec7d to b93ae62 Compare March 12, 2019 18:25
This prevents on-disk inconsistencies when a node crashes in the
middle of a merge and the lease applied index temporarily regresses.

Fixes cockroachdb#33120

Release note (bug fix): Fixed an on-disk inconsistency that could
result from a crash during a range merge.
@bdarnell bdarnell force-pushed the merge-waitforapplication branch from b93ae62 to 68e34eb Compare March 12, 2019 18:43
@bdarnell
Copy link
Contributor Author

bors r=tbg

craig bot pushed a commit that referenced this pull request Mar 12, 2019
35626: storage: Sync to disk before returning in WaitForApplication r=tbg a=bdarnell

This prevents on-disk inconsistencies when a node crashes in the
middle of a merge and the lease applied index temporarily regresses.

Fixes #33120

Release note (bug fix): Fixed an on-disk inconsistency that could
result from a crash during a range merge.


The second commit is a manual test (adapted from @tbg) that demonstrates the "overlapping range" failure if the WriteSyncNoop call is removed, and also shows that this call fixes the problem. It's obviously not mergeable in its current state, but I'm not sure how to test this without some gross hacks. 

Co-authored-by: Ben Darnell <[email protected]>
@craig
Copy link
Contributor

craig bot commented Mar 12, 2019

Build succeeded

@craig craig bot merged commit 68e34eb into cockroachdb:master Mar 12, 2019
@bdarnell bdarnell deleted the merge-waitforapplication branch March 12, 2019 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage: chaos during merges leads to overlapping range descriptors
3 participants